Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hmr): extend acceptHMRUpdate to allow for cleaning up side effects of existing store #2793

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

Anoesj
Copy link

@Anoesj Anoesj commented Oct 4, 2024

Changes

This PR adds to possibility of passing a cleanup function as an optional third argument to acceptHMRUpdate. This cleanup function can be used to clean up side effects of the existing store before initializing the new store. This is useful if you have event listeners or other side effects that need to be cleaned up in the old store.

This solves a non-trivial case. Usually, having side effects inside a Pinia store is probably not such a great idea. However, it happens, and if it does, we need to provide a way to make sure those side effects can be properly cleaned up during HMR.

Let's say you have an app that relies on a lot of event listeners, and you want to manage them centrally and close to some global state. If there isn't an obvious Vue component to add and remove all these event listeners, you might choose to manage these event listeners in a Pinia store. While working on this Pinia store, you might need to clean up some of those event listeners, before HMR replaces the existing store with the new one. We can use onScopeDispose to run some code when the store is disposed, but nothing is disposed when HMR swaps the stores, so we can't use that mechanism here. For better control over HMR, we need a way to talk to the existing store, just before it gets replaced. This PR provides that mechanism.

To do

  • Tests: I couldn't find a way to properly test this. If someone can assist with that, that would be nice! I've manually tested using the playground (see the /scroll-store route).
  • Translations: I updated the English docs about HMR, but the Chinese version needs to be updated as well.
  • Explore alternatives: I can imagine something like an onBeforeHMRReplace hook being a viable alternative. A user would need to call onBeforeHMRReplace in a setup store, just like with onScopeDispose. It would probably require us to detect and register any called onBeforeHMRReplace hooks during the setup phase (store initialization) and manually trigger their callbacks in acceptHMRUpdate.

Example (taken from the modified docs)

// scroll.js
import { defineStore, acceptHMRUpdate } from 'pinia'
import { ref } from 'vue'

export const useScroll = defineStore('scroll', () => {
  const scrollTop = ref(window.scrollY)

  function onScroll () {
    scrollTop.value = window.scrollY
  }

  function trackScroll () {
    window.addEventListener('scroll', onScroll, { passive: true })
  }

  trackScroll()

  function $cleanUp () {
    window.removeEventListener('scroll', onScroll)
  }

  return {
    scrollTop,
    trackScroll,
    $cleanUp,
  }
})

if (import.meta.hot) {
  import.meta.hot.accept(acceptHMRUpdate(useScroll, import.meta.hot, (existingStore) => {
    existingStore.$cleanUp()
  }))
}

Copy link

netlify bot commented Oct 4, 2024

Deploy Preview for pinia-playground ready!

Name Link
🔨 Latest commit fa11794
🔍 Latest deploy log https://app.netlify.com/sites/pinia-playground/deploys/66fffce68e073d000833b343
😎 Deploy Preview https://deploy-preview-2793--pinia-playground.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 4, 2024

Deploy Preview for pinia-official ready!

Name Link
🔨 Latest commit fa11794
🔍 Latest deploy log https://app.netlify.com/sites/pinia-official/deploys/66fffce62a97c00008d76432
😎 Deploy Preview https://deploy-preview-2793--pinia-official.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should simply dispose of the old store to trigger onScopeDispose() just like components trigger onUnmounted(). There could be limitations but I'm more keen on exploring that side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 Triaging
Development

Successfully merging this pull request may close these issues.

2 participants